Fix AutoRegisterRequestProcessors to include all implementations #989
Conversation
|
Will this double-register processors? |
|
I don't believe so. I didn't want to make the tests too brittle by asserting based upon the Pre/Post-processors currently defined. preProcessors.Count.ShouldBe(4);But then if another PreProcessor is added to cover another test case this test will break. Without the fix the second assertion here fails - because it only registers the first type: preProcessors.ShouldContain(p => p != null && p.GetType() == typeof(FirstConcretePreProcessor));
preProcessors.ShouldContain(p => p != null && p.GetType() == typeof(NextConcretePreProcessor)); // failI'm happy to change the tests here to make them more explicit - just be aware this will make them potentially more brittle. Looking at the code: The only way I can see a sort of duplicate registration would be if you had something like this: public class Processor1 : IRequestPreProcessor<Ping>
{
}
public class Processor2 : Processor1, IRequestPreProcessor<Ping>
{
}But I can't think of a legitimate reason to do that anyway. |
|
Hi @jbogard, I agree with @hisuwh's claim that there is a bug in the way the new // <PackageReference Include="MediatR" Version="12.2.0" />
// <PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="8.0.0" />
using MediatR;
using MediatR.Pipeline;
using Microsoft.Extensions.DependencyInjection;
var services = new ServiceCollection().AddMediatR(x =>
{
x.AutoRegisterRequestProcessors = true;
x.RegisterServicesFromAssemblyContaining<Program>();
});
await using var serviceProvider = services.BuildServiceProvider();
await serviceProvider.GetRequiredService<IMediator>().Send(new MyRequest());
public class MyRequest : IRequest;
public class MyRequestHandler : IRequestHandler<MyRequest>
{
public Task Handle(MyRequest request, CancellationToken cancellationToken) => Task.CompletedTask;
}
public class MyRequestProcessor : IRequestPreProcessor<MyRequest>, IRequestPostProcessor<MyRequest, Unit>
{
public Task Process(MyRequest request, CancellationToken cancellationToken) => Task.CompletedTask;
public Task Process(MyRequest request, Unit response, CancellationToken cancellationToken) => Task.CompletedTask;
}It's very unfortunate as we were about to leverage pre/post processors in one of my team's features. Our only option right now is to downgrade MediatR to 12.0.1, which involves not benefiting from the recent improvements and other bug fixes. |
|
+1 for "this is impacting me" |
|
@jbogard do you want me to change anything on this or add any more tests? I'm happy to do so. |
|
That scanning stuff is code I copied from some other DI projects, so you can see why I don't really want to copy more behaviors from DI containers because that code is...complicated |
|
@jbogard when are you planning new release with this in please 🙏🥺 |
[](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [MediatR](https://togithub.com/jbogard/MediatR) | `12.2.0` -> `12.3.0` | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | [](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>jbogard/MediatR (MediatR)</summary> ### [`v12.3.0`](https://togithub.com/jbogard/MediatR/releases/tag/v12.3.0) #### What's Changed - Fix AutoRegisterRequestProcessors to include all implementations by [@​hisuwh](https://togithub.com/hisuwh) in [https://github.com/jbogard/MediatR/pull/989](https://togithub.com/jbogard/MediatR/pull/989) - [#​1016](https://togithub.com/jbogard/MediatR/issues/1016) Use repo readme for base package by [@​thompson-tomo](https://togithub.com/thompson-tomo) in [https://github.com/jbogard/MediatR/pull/1017](https://togithub.com/jbogard/MediatR/pull/1017) - Add Support for Generic Handlers by [@​zachpainter77](https://togithub.com/zachpainter77) in [https://github.com/jbogard/MediatR/pull/1013](https://togithub.com/jbogard/MediatR/pull/1013) #### New Contributors - [@​hisuwh](https://togithub.com/hisuwh) made their first contribution in [https://github.com/jbogard/MediatR/pull/989](https://togithub.com/jbogard/MediatR/pull/989) - [@​thompson-tomo](https://togithub.com/thompson-tomo) made their first contribution in [https://github.com/jbogard/MediatR/pull/1017](https://togithub.com/jbogard/MediatR/pull/1017) - [@​zachpainter77](https://togithub.com/zachpainter77) made their first contribution in [https://github.com/jbogard/MediatR/pull/1013](https://togithub.com/jbogard/MediatR/pull/1013) **Full Changelog**: jbogard/MediatR@v12.2.0...v12.3.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 7am on Sunday,before 7am on Wednesday" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/digdir/dialogporten). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zOTMuMCIsInVwZGF0ZWRJblZlciI6IjM3LjM5My4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Ole Jørgen Skogstad <skogstad@softis.net>
](https://renovatebot.com){kind=link}
|
I thought this change would fix this very simple usecase but it still doesn't work with 12.3.0: |
This is how this previously worked. When this option was restored the
addIfAlreadyExistsproperty was mistakenly set to false.I have updated the tests and changed this property.